[TRTLLM-13233][feat] Support no_repeat_ngram_size in TorchSampler and refactor token-ban handling into its own submodule#16594
Conversation
no_repeat_ngram_size in TorchSampler
no_repeat_ngram_size in TorchSamplerno_repeat_ngram_size in TorchSampler
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Torch executor now propagates ChangesNo-repeat n-gram sampling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExecutorRequest
participant LlmRequest
participant TorchSampler
participant Logits
ExecutorRequest->>LlmRequest: propagate no_repeat_ngram_size
LlmRequest->>TorchSampler: provide request context and ngram size
TorchSampler->>TorchSampler: collect bad-word and no-repeat n-gram bans
TorchSampler->>Logits: apply accumulated token bans
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/sampler/test_torch_sampler.py (1)
2789-2797: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding a multi-beam coverage case.
All
_run/_run_staleinvocations usenum_beams == 1, so the multi-beam branch of_collect_no_repeat_ngram_bans.fresh_rules(thenum_beams[index] != 1path that scans each beam's context directly rather than the incremental index) is never exercised. Since beam search is a supported production path, a follow-up test that passesnum_beams > 1with divergent per-beam histories would close this gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/sampler/test_torch_sampler.py` around lines 2789 - 2797, Add a focused multi-beam test using the existing _run or _run_stale helpers, passing num_beams greater than one with distinct histories for each beam. Assert the no-repeat n-gram bans for every beam, exercising _collect_no_repeat_ngram_bans.fresh_rules through its num_beams[index] != 1 path while preserving existing single-beam coverage.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unittest/_torch/sampler/test_torch_sampler.py`:
- Around line 2789-2797: Add a focused multi-beam test using the existing _run
or _run_stale helpers, passing num_beams greater than one with distinct
histories for each beam. Assert the no-repeat n-gram bans for every beam,
exercising _collect_no_repeat_ngram_bans.fresh_rules through its
num_beams[index] != 1 path while preserving existing single-beam coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8beb9dee-254c-4001-aea5-a5e5f79b7bfe
📒 Files selected for processing (5)
docs/source/features/sampling.mdtensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/sampler/sampler.pytensorrt_llm/sampling_params.pytests/unittest/_torch/sampler/test_torch_sampler.py
7bf7eb4 to
940c68e
Compare
|
Hi @lori-ren all comments were addressed, please take a look. |
940c68e to
646e258
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/_torch/sampler/test_torch_sampler.py (2)
2954-3024: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFresh-path assertions verified correct.
Manually traced the index construction against all bigram/trigram/unigram/incremental/rollback/multi-step assertions in this block — all match the expected
_extend_no_repeat_ngram_indexsemantics.One gap:
test_disabled_request_untouched(Line 2989-2994) only coversngram_size=None. TheSamplingParams.no_repeat_ngram_sizecontract states bothNoneand0disable the restriction — worth adding a0case alongsideNoneto lock in that branch, since0exercises a different code path (an explicit falsy check) thanNone.✅ Suggested additional case
def test_disabled_request_untouched(self): active = self.MockLlmRequest(tokens=[1, 2, 3, 1, 2]) - disabled = self.MockLlmRequest(tokens=[1, 2, 3, 1, 2]) - logits = self._run([active, disabled], [2, None]) + disabled_none = self.MockLlmRequest(tokens=[1, 2, 3, 1, 2]) + disabled_zero = self.MockLlmRequest(tokens=[1, 2, 3, 1, 2]) + logits = self._run([active, disabled_none, disabled_zero], [2, None, 0]) assert self._banned_cols(logits[0]) == {3} - assert self._banned_cols(logits[1]) == set() + assert self._banned_cols(logits[1]) == set() + assert self._banned_cols(logits[2]) == set()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/sampler/test_torch_sampler.py` around lines 2954 - 3024, Add coverage to test_disabled_request_untouched for an explicitly configured no_repeat_ngram_size of 0, alongside the existing None case. Verify the request remains unchanged and no tokens are banned, while preserving the current enabled-request assertion.
3025-3105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale-host assertions verified correct; multi-beam divergence untested.
Traced the stale/device-boundary math for all 8 stale-path tests (including the subtle "virtual self-transition" case in
test_stale_no_nan_on_duplicate_bans) — all check out.
_make_sampler(Line 2926-2937) fixesmax_beam_width=1, and no test here passesnum_beams > 1with per-beam-divergent histories. Per_extend_no_repeat_ngram_index's own docstring, the incremental cache is single-beam only "as beam histories diverge," implying a separate/fallback code path handlesbeam_width > 1. That path isn't exercised in this file, despite the PR description listing "multi-beam layouts" among the 18 covered scenarios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/sampler/test_torch_sampler.py` around lines 3025 - 3105, Add coverage for divergent multi-beam histories in the stale-host sampler tests; current _make_sampler fixes max_beam_width=1 and never exercises the fallback path. Create a sampler/request setup with num_beams greater than one and distinct per-beam token histories, then invoke _run_stale or the relevant sampling flow and assert each beam receives the correct no-repeat-ngram bans. Ensure the test validates the path used when _extend_no_repeat_ngram_index cannot use its single-beam incremental cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unittest/_torch/sampler/test_torch_sampler.py`:
- Around line 2954-3024: Add coverage to test_disabled_request_untouched for an
explicitly configured no_repeat_ngram_size of 0, alongside the existing None
case. Verify the request remains unchanged and no tokens are banned, while
preserving the current enabled-request assertion.
- Around line 3025-3105: Add coverage for divergent multi-beam histories in the
stale-host sampler tests; current _make_sampler fixes max_beam_width=1 and never
exercises the fallback path. Create a sampler/request setup with num_beams
greater than one and distinct per-beam token histories, then invoke _run_stale
or the relevant sampling flow and assert each beam receives the correct
no-repeat-ngram bans. Ensure the test validates the path used when
_extend_no_repeat_ngram_index cannot use its single-beam incremental cache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dcf99299-557a-45d6-b3fa-5a910e2cba56
📒 Files selected for processing (5)
docs/source/features/sampling.mdtensorrt_llm/_torch/pyexecutor/llm_request.pytensorrt_llm/_torch/pyexecutor/sampler/sampler.pytensorrt_llm/sampling_params.pytests/unittest/_torch/sampler/test_torch_sampler.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tensorrt_llm/sampling_params.py
- docs/source/features/sampling.md
- tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
dcead0f to
58f6d9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unittest/_torch/sampler/test_torch_sampler.py (1)
3002-3007: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the
0disabled value.The contract requires both
Noneand0to disable the restriction, but this test only exercisesNone. Add a second disabled request usingngram_size=0and assert that its logits remain unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/sampler/test_torch_sampler.py` around lines 3002 - 3007, Extend test_disabled_request_untouched to include a request configured with ngram_size=0, run it alongside the existing active and None-disabled requests, and assert its banned columns are empty. Preserve the existing assertions for the active request and the None-disabled request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/_torch/sampler/test_torch_sampler.py`:
- Around line 2952-2960: Extend the tests around _run and _apply_no_repeat_ngram
with a num_beams=[2] case using distinct token histories for each beam.
Configure the mock request so get_tokens(beam_idx) returns beam-specific
sequences, then assert each logits row is banned only according to its
corresponding beam history.
---
Outside diff comments:
In `@tests/unittest/_torch/sampler/test_torch_sampler.py`:
- Around line 3002-3007: Extend test_disabled_request_untouched to include a
request configured with ngram_size=0, run it alongside the existing active and
None-disabled requests, and assert its banned columns are empty. Preserve the
existing assertions for the active request and the None-disabled request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a326faf9-9f4a-43d5-a483-72fee52fadd5
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/sampler/sampler.pytests/unittest/_torch/sampler/test_torch_sampler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
646e258 to
dcead0f
Compare
Implement the no_repeat_ngram_size sampling parameter for the PyTorch TorchSampler (previously honored only by the C++ executor path). A token is excluded from sampling if it would recreate an n-gram already present in the sequence (prompt included), matching the C++ banRepeatNgram kernel and HF's NoRepeatNGramLogitsProcessor. None or 0 disables the restriction. Token-ban handling is organized into a dedicated token_ban submodule: - TokenBans: host-side accumulator of (row, col) index lists; all features append into one instance and flush with a single H2D transfer + index_put_ per category. - TokenBanHandler (base): shared feature logic (bad words, no-repeat ngram, min-length EOS suppression, the suffix-rule matcher, the incremental n-gram index, and the unconditional flush). - Two variants selected once from whether the overlap scheduler is enabled: SynchronousTokenBanHandler (history always complete, all bans unconditional) and OverlappedTokenBanHandler (a batch mixes fresh and stale requests; stale ones produce conditional bans resolved on the device without a D2H sync). bad_words and min-length EOS suppression are folded into the same module and share the single accumulator/flush. The n-gram matcher keeps an incremental per-request index (host-side token mirror grown via scalar accessors, avoiding the ~40us full get_tokens() copy), so the steady-state per-step cost is O(new tokens); the isolated sampler benchmark shows ~2us n-gram overhead on H200 (bs=32, 2048-token context, n=3). Docs and the SamplingParams docstring updated. Unit tests for all three token-ban paths live in tests/unittest/_torch/sampler/test_token_ban.py. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>
81dcd3c to
5f44c53
Compare
no_repeat_ngram_size in TorchSampler|
Hi @ixlmar I have refactored all token-ban handling into a new token_ban submodule:
All three token-ban features — bad words, no-repeat ngram, and the min-length EOS suppression — are now merged into this one handler and share the single accumulator and flush. Token-ban unit tests moved to a dedicated test_token_ban.py. No behavior or measurable performance change. |
Summary
Dev Engineer Review
no_repeat_ngram_sizesupport toTorchSampler, aligning with C++banRepeatNgramand Hugging Face behavior:n-gram in the generated sequence including prompt tokens.n == 1, bans all previously seen tokens (unigram repeat prevention).Noneor0._apply_token_bans._TokenBansto accumulate ban targets across unconditional, overlap-conditioned, and device-token-column cases._NgramIndexCachefor efficient updates.neg_inftensor for overlap conditional penalties to avoid extra allocations._process_requeststo compute per-requestngram_sizesand apply both features via shared collection/flush when overlap scheduler tracking is enabled._NgramIndexCacheinWeakKeyDictionary[LlmRequest, _NgramIndexCache]so state is reclaimed when requests are freed (including rollback scenarios).executor_request_to_llm_requestnow readsexecutor_request.sampling_config.no_repeat_ngram_size, normalizes falsy/disabled values toNone, and setsllm_request.py_no_repeat_ngram_size.docs/source/features/sampling.mdto document prompt-inclusive n-gram banning and thatNone/0disables the restriction.SamplingParamsdocstring to explicitly describe the prompt-inclusive behavior and the default (None).QA Engineer Review
tests/unittest/_torch/sampler/test_torch_sampler.py:TestApplyBadWordsto use a new sampler helper for both fresh and stale-host paths.TestApplyNoRepeatNgramwith aMockLlmRequestand helpers to validate banned-logit columns and correct index/cache behavior.tests/integration/test_lists/,test-db/, orqa/: not verifiable from the provided context (no test-list changes available).Description
This PR does two things:
no_repeat_ngram_sizefor the PyTorchTorchSampler. Theparameter was previously honored only by the C++ executor path and silently
ignored on the PyTorch backend.
min-length EOS suppression) out of the large
sampler.pyinto a dedicatedtoken_bansubmodule with a small, testable class hierarchy.no_repeat_ngram_sizesemanticsA token is excluded from sampling if it would recreate an n-gram already
present in the sequence (prompt included) — same semantics as the C++
banRepeatNgramkernel and HF'sNoRepeatNGramLogitsProcessor.n == 1bansevery token already present; the restriction is per-beam.
Noneor0disables it. Docs (
docs/source/features/sampling.md) and theSamplingParamsdocstring are updated accordingly.
Token-ban submodule (
token_ban.py)Bad words, no-repeat ngram, and min-length EOS suppression are all the same
shape of operation — mask specific logits to
-inf. They now share one module:TokenBans— host-side accumulator of(row, col)index lists. Everyfeature appends into a single instance, so all bans reach the device with one
H2D transfer + one
index_put_per category (instead of one set per feature).TokenBanHandler(ABC) — holds the shared feature logic: the bad-words /no-repeat-ngram rule generators, the suffix-rule matcher, the incremental
per-request n-gram index, min-length collection, and the unconditional flush.
scheduler is enabled (a fixed config choice, not per-request):
SynchronousTokenBanHandler— overlap off; the host history is alwayscomplete, so every ban is unconditional.
OverlappedTokenBanHandler— overlap on; a batch mixes fresh requests(history complete) with stale ones (missing the previous step's token,
still on the device). Stale requests produce conditional bans resolved on
the device at apply time (
torch.whereon the pending token) without adevice-to-host sync.
TorchSamplernow just owns a handler and callsgenerate_ban_list()/apply_ban_list(); it computes the per-request staleflags since it owns the pending-step / draft-batch context.
Performance
The n-gram matcher keeps an incremental per-request index rather than
rescanning the history each step: a host-side token mirror is grown via the
scalar
get_num_tokens/get_last_tokensaccessors, avoiding the ~40 µs fullget_tokens()copy, so the steady-state per-step cost is O(new tokens).Isolated sampler benchmark (H200, bs=32, 2048-token context per request, n=3,
p50 sampler-beyond-forward):
The submodule refactor and folding in min-length introduced no measurable
change vs the pre-refactor numbers.
Overlap + speculation / beam search
As with bad words, when the overlap scheduler is combined with speculative
decoding or beam search the missing host tokens cannot be reconstructed with
the single-token device check, so bans may be enforced one step late (warned
once). The exact stale path only runs for single-step, single-beam requests.
Test Coverage
tests/unittest/_torch/sampler/test_token_ban.py(new file) covers all threetoken-ban paths against the handlers directly:
prefixes, and the full stale-host overlap matrix.
n == 1,sequences shorter than
n, disabled (Noneand0), multi-step andmulti-beam (per-beam history) layouts, the stale overlap matrix (device
hit/miss, window ending at the device token, mixed stale+fresh, no NaN on
duplicate bans), incremental cache growth, and cache rebuild on rollback.
the prompt), no ban once reached, invalid end_id skipped, multi-step boundary.
Tests assert that logits outside the banned set are left unchanged (starting
from distinct non-zero logits), not just that the banned cells become
-inf.All pass on H200. Additional randomized fuzz (matched against a brute-force
reference and against the "fresh path on the completed sequence" for the stale
path) was run during development.
PR Checklist
[JIRA/NVBUG/None][type] descriptionformatdocs/source/features/sampling.md,SamplingParamsdocstring)PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.